-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Allow external structs. #2798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Allow external structs. #2798
Conversation
2eb1e7b to
0b0aac0
Compare
0b0aac0 to
410d4a3
Compare
dd47c44 to
f6e272f
Compare
410d4a3 to
687192d
Compare
f6e272f to
5ec993c
Compare
687192d to
b891a9d
Compare
Specifically, 1. Introduce a PlaintextType::ExternalStruct, and allow referring to such a type in .aleo code by a Locator. 2. Allow casting to external struct types. 3. Let two struct types be considered the same type as long as they are structurally the same.
b891a9d to
558625f
Compare
|
Rebased. |
vicsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a check in check_transaction that deployments using an external struct are only enabled from ConsensusVersion::V10 onwards. You can put in dummy heights for the new consensus version.
And while you're at it, consider refactoring ConsensusVersion-related checks into a separate function called by check_transaction?
|
I moved some of the consensus version checks into a new function, but some others are scattered throughout |
vicsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
I would appreciate a short walk through synthesizer/program/src/logic/instruction/operation/cast.rs during the next Leo Weekly.
I'll defer to @d0cd about the usage and implementation of types_structurally_equivalent and whether he seed potential impending issues related to dynamic dispatch.
Added a failing test using a non-existant external struct.
|
Here's some updates. You also asked me to walk you through the changes in
|
|
Can you make a PR to add clarification to the docs:
|
|
Can you use this to confirm that the R1CS for all known deployments remains unchanged? https://github.com/ProvableHQ/aleo-program-regressions/tree/master/r1cs-hasher EDIT: It's been a while since we touched it, so may need some minor refactoring. Specifically, I see it is unclear which deployments have been used for the previous hash, I assume the first 211 of either canary/testnet/mainnet because the hash file name ends with an inexplicable |
Sure. |
I can't get this to work on recent snarkvm commits. Using snarkvm UPDATE written by Victor: the tests passed now. |
|
Regarding the docs about all the commands/instructions that compare equality of structs. Here's the actual behavior (both before and after this PR; it's unchanged):
I think this is not really behavior we want users to think about, but instead just want to users to only try to compare two struct values of the same type. For that reason I'm inclined to leave the documentation of those instructions as is, but let me know if you feel otherwise. |
Change tag for ArrayType element type. Introduce and use finalize_types_structurally_equivalent. Use types_structurally_equivalent in synthesizer/process/src/stack/finalize_types/matches.rs `ProgramID` with backticks in comments.
|
@d0cd I've checked your questions again; I added a couple comments but I think the code as of the last push had addressed all the issues you raised. Let me know what you think. |
synthesizer/src/vm/verify.rs
Outdated
| ); | ||
| } | ||
|
|
||
| let stack = self.process.read().get_stack(deployment.program_id())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the stack guaranteed to exist at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooff good catch. No it's not. And we don't even need it, we just read the program from it. We should add some integration tests in a new test_v11.rs file. You can copy behaviour from test_v10.rs and test_v9.rs: e.g. simply create a transaction with external struct before the V11 height, should fail to verify, after the V11 height, should succeed to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good grief I'm a mess; sorry. I actually already fixed this locally before I even pushed this but somehow pushed the wrong commit. I've now pushed the fix, along with the changes to comments. I'll add the test_v11.rs shortly.
synthesizer/src/vm/verify.rs
Outdated
| // If the `CONSENSUS_VERSION` is greater than or equal to `V9`, then verify that: | ||
| // - the program checksum is present in the deployment | ||
| // - the program owner is present in the deployment | ||
| // If the `CONSENSUS_VERSION` is less than `V10`, then verify that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // If the `CONSENSUS_VERSION` is less than `V10`, then verify that: | |
| // If the `CONSENSUS_VERSION` is less than `V11`, then verify that: |
synthesizer/src/vm/verify.rs
Outdated
| // - the program checksum is present in the deployment | ||
| // - the program owner is present in the deployment | ||
| // If the `CONSENSUS_VERSION` is less than `V10`, then verify that: | ||
| // - the program does not use the external struct syntax `some_program.aleo/StructT` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // - the program does not use the external struct syntax `some_program.aleo/StructT` | |
| // - the program does not use the external struct syntax `some_program.aleo/StructT` | |
| // - the program's mappings do not use non-existent structs. |
| } | ||
| // Ensure the operands are of the same type. | ||
| if input_types[0] != input_types[1] { | ||
| if !register_types_structurally_equivalent(stack, &input_types[0], stack, &input_types[1])? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forking risk, transaction validity changed
Also change type equivalence to require structs have the same name.
- external_struct_fail_name test - `plaintext_exists` moved to `Process` as `mapping_types_exist`.
|
@mikebenfield one of the synthesizer tests halted with a mysterious Note that a small improvement to
|
|
@vicsn Alright I fixed that test but unfortunately didn't enhance the test function. There's now a semver check failing, but I don't think that is related to this PR. |
|
@mohammadfawaz can records contain structs and/or external structs? Do we have tests for this in place? CC @Antonio95 for his work on dynamic dispatch |
Specifically,
Introduce a PlaintextType::ExternalStruct, and allow referring to such a type in .aleo code by a Locator.
Allow casting to external struct types.
Let two struct types be considered the same type as long as they are structurally the same and have the same local names. This funky ruleset is effectively here for backwards compatibility. In more detail:
Also note this other usecase from a user: